refactor(e2e): migrate the tests to the new knuu #3873
Conversation
mojtaba-esk
requested review from
staheri14 and
rach-id
and removed request for
a team
WalkthroughWalkthroughThe pull request introduces substantial modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Testnet
participant Knuu
User->>Testnet: New(ctx)
Testnet->>Knuu: Initialize(options)
Testnet->>User: Testnet Instance Ready
User->>Testnet: CreateGenesisNode(ctx)
Testnet->>Knuu: Create Node with Context
Knuu-->>Testnet: Node Created
Testnet-->>User: Genesis Node Created
User->>Testnet: Cleanup(ctx)
Testnet->>Knuu: Cleanup Instance
Knuu-->>Testnet: Instance Cleaned
Assessment against linked issues
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
test/e2e/minor_version_compatibility.go (1)
20-20: Remove the unused parameter.The additional string parameter
_in the function signature is unused. Consider removing it to keep the function signature clean and concise.Apply this diff to remove the unused parameter:
-func MinorVersionCompatibility(logger *log.Logger, _ string) error { +func MinorVersionCompatibility(logger *log.Logger) error {
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (12)
- go.mod (7 hunks)
- test/e2e/benchmark/benchmark.go (5 hunks)
- test/e2e/benchmark/throughput.go (4 hunks)
- test/e2e/main.go (4 hunks)
- test/e2e/major_upgrade_v2.go (2 hunks)
- test/e2e/minor_version_compatibility.go (3 hunks)
- test/e2e/simple.go (1 hunks)
- test/e2e/testnet/defaults.go (1 hunks)
- test/e2e/testnet/node.go (13 hunks)
- test/e2e/testnet/setup.go (2 hunks)
- test/e2e/testnet/testnet.go (20 hunks)
- test/e2e/testnet/txsimNode.go (5 hunks)
Additional context used
GitHub Check: lint / golangci-lint
test/e2e/main.go
[failure] 25-25:
ineffectual assignment to latestVersion (ineffassign)
Additional comments not posted (77)
test/e2e/testnet/defaults.go (1)
6-9: LGTM! The changes improve type safety and align with best practices.The modifications to the
DefaultResourcesvariable, replacing string literals with calls toresource.MustParse, offer several benefits:
Enhanced type safety: By using
resource.MustParse, the resource values are correctly parsed into the appropriate types expected by Kubernetes APIs, reducing the risk of type-related errors.Improved validation and handling: The changes allow for better validation and handling of resource specifications, ensuring that the values are properly formatted and within acceptable ranges.
Alignment with best practices: The use of
resource.MustParsealigns with best practices for resource management in Kubernetes environments, promoting consistency and maintainability.Overall, these changes contribute to a more robust and reliable implementation of resource specifications in the testnet package.
test/e2e/testnet/txsimNode.go (12)
5-5: LGTM!The import of the
contextpackage aligns with the PR objective of enhancing context handling and is consistent with the AI-generated summary.
8-8: LGTM!The import of the
instancepackage aligns with the PR objective of refactoring to eliminate deprecated methods from theknuupackage and is consistent with the AI-generated summary.
23-23: LGTM!The modification of the
Instancefield in theTxSimstruct to use*instance.Instancealigns with the PR objective of refactoring to eliminate deprecated methods from theknuupackage and is consistent with the AI-generated summary.
27-27: LGTM!The addition of the
context.Contextparameter to theCreateTxClientfunction aligns with the PR objective of enhancing context handling and is consistent with the AI-generated summary.
37-37: LGTM!The addition of the
*knuu.Knuuparameter to theCreateTxClientfunction aligns with the PR objective of refactoring to eliminate deprecated methods from theknuupackage.
39-39: LGTM!The creation of a new instance using
knuu.NewInstance(name)aligns with the PR objective of refactoring to eliminate deprecated methods from theknuupackage and is consistent with the AI-generated summary.
48-48: LGTM!The use of
txIns.Build().SetImage(ctx, image)to set the image for the instance aligns with the PR objective of refactoring to eliminate deprecated methods from theknuupackage and is consistent with the AI-generated summary. The addition of thectxparameter also aligns with the PR objective of enhancing context handling.
56-56: LGTM!The use of
txIns.Resources().SetMemory(resources.MemoryRequest, resources.MemoryLimit)to set the memory resources for the instance aligns with the PR objective of refactoring to eliminate deprecated methods from theknuupackage and is consistent with the AI-generated summary.
60-60: LGTM!The use of
txIns.Resources().SetCPU(resources.CPU)to set the CPU resources for the instance aligns with the PR objective of refactoring to eliminate deprecated methods from theknuupackage and is consistent with the AI-generated summary.
64-64: LGTM!The use of
txIns.Storage().AddVolumeWithOwner(volumePath, resources.Volume, 10001)to add a volume to the instance aligns with the PR objective of refactoring to eliminate deprecated methods from theknuupackage and is consistent with the AI-generated summary.
78-78: LGTM!The use of
txIns.Build().SetArgs(args...)to set the arguments for the instance aligns with the PR objective of refactoring to eliminate deprecated methods from theknuupackage and is consistent with the AI-generated summary.
84-84: LGTM!The return of a
TxSimstruct with theInstancefield set totxInsaligns with the PR objective of refactoring to eliminate deprecated methods from theknuupackage and is consistent with the AI-generated summary.test/e2e/main.go (4)
15-15: LGTM!The modification to the
TestFunctype to accept aversionparameter aligns with the PR objective of migrating the tests to the new Knuu framework. This change can improve the flexibility and clarity of the testing process by explicitly associating tests with the application version they are intended to validate.
25-31: Changes look good! The hardcoded version is a valid temporary workaround.The changes to retrieve and log the application version align with the PR objective of migrating the tests to the new Knuu framework.
Regarding the hardcoded version on line 29, I acknowledge the past review comment indicating that it's a temporary workaround for a known issue with BBR on Kubernetes. The TODO comment appropriately indicates the intent to remove this workaround once the underlying issue is resolved.
Please ignore the static analysis hint about the ineffectual assignment to
latestVersion. It's a false positive becauselatestVersionis used in subsequent code.Tools
GitHub Check: lint / golangci-lint
[failure] 25-25:
ineffectual assignment to latestVersion (ineffassign)
44-44: LGTM!The update to the
runTestinvocation to pass thelatestVersionto each test aligns with the PR objective of migrating the tests to the new Knuu framework. This change ensures that the tests have access to the correct application version during execution.
62-64: LGTM!The modifications to the
runTestfunction to accept anappVersionparameter and pass it to the test function align with the PR objective of migrating the tests to the new Knuu framework. These changes ensure that the tests have access to the correct application version during execution.test/e2e/simple.go (5)
17-17: LGTM!The addition of the
appVersionparameter to the function signature is a good change. It allows for more flexibility in specifying the application version directly when calling the function, which is consistent with the PR objective of migrating the tests to the new knuu framework.
18-18: Great job with the context management improvements!The introduction of the
context.Contextparameter,ctx, and passing it to various method calls within thetestNetobject is a great improvement. It enhances the handling of cancellation and deadlines for the operations, which is consistent with the best practices for context management in Go. Also, deferring thetestNet.Cleanupmethod with thectxparameter ensures proper cleanup even if an error occurs.Also applies to: 22-22, 25-26
36-36: LGTM!Passing the
ctxparameter to thetestNet.SetupandtestNet.Startmethods is consistent with the context management improvements. It ensures that the setup and start operations respect the context cancellation and deadlines.Also applies to: 39-39
45-45: LGTM!Passing the
ctxparameter to thetestnode.ReadBlockchainHeadersfunction is consistent with the context management improvements. It ensures that the operation of reading blockchain headers respects the context cancellation and deadlines.
29-29: Verify the assumption about the endpoints slice.The changes to pass the
ctxparameter to thetestNet.RemoteGRPCEndpointsandtestNet.CreateTxClientmethods are consistent with the context management improvements.However, using the first element of the
endpointsslice assumes that there is at least one endpoint available. Please verify this assumption to ensure that it holds true in all scenarios.You can use the following script to verify the assumption:
Also applies to: 31-31
test/e2e/testnet/setup.go (1)
16-23: LGTM!The addition of the
context.Contextparameter is a good practice for managing cancellation and timeouts in the function. The parameter is correctly propagated to thenode.AddressP2Pmethod, which allows the method to respect any cancellation or timeout signals from the caller. The changes look good to me.test/e2e/minor_version_compatibility.go (2)
37-40: LGTM: Context management changes.The introduction of context parameters in various function calls and the corresponding changes to the testnet creation, preloader methods, error handling, and upgrade process to utilize the context are crucial for improving the handling of cancellation, timeouts, and proper lifecycle management. These changes enhance the function's robustness and responsiveness.
Also applies to: 45-45, 48-48, 50-50, 57-59, 63-63, 65-65, 70-70, 72-72, 88-88, 91-91
91-91: LGTM: Timeout adjustment.Extending the timeout duration in the
waitForHeightfunction is a more robust approach to waiting for node synchronization after upgrades. This change helps ensure that the nodes have sufficient time to reach the expected height, reducing the chances of false positives in the compatibility test.test/e2e/major_upgrade_v2.go (1)
Line range hint
18-97: Excellent work on refactoring theMajorUpgradeToV2function!The changes made to the function significantly improve its flexibility, robustness, and resource management. Key improvements include:
- The introduction of the
appVersionparameter, which allows for better version management during the upgrade process.- The consistent use of context for various operations, enhancing the function's ability to handle cancellations and timeouts gracefully.
- The modifications to the upgrade process for each node, ensuring proper management of long-running operations and appropriate release of resources.
- Updating the method for adding Docker images to the preloader to use the
appVersionparameter, aligning image management with the specified application version and enhancing reliability and predictability during upgrades.These changes align perfectly with the objectives outlined in the PR summary and the linked issue #3780, which aim to stop using deprecated Knuu methods and enable staticcheck in the e2e package. The refactoring efforts contribute to the overall goal of modernizing the codebase and improving the robustness of the end-to-end testing framework.
test/e2e/benchmark/throughput.go (14)
4-4: LGTM!The import of the
contextpackage is necessary for the changes made to introduce context parameters in various functions.
10-10: LGTM!The import of the
resourcepackage fromk8s.io/apimachinery/pkg/apiis necessary for the changes made to useresource.MustParsefor resource specifications.
22-22: LGTM!The change to use
resource.MustParse("12Gi")for theMemoryRequestfield in theValidatorResourcestructure enhances type safety and ensures that the memory request is correctly parsed and validated.
23-23: LGTM!The change to use
resource.MustParse("12Gi")for theMemoryLimitfield in theValidatorResourcestructure enhances type safety and ensures that the memory limit is correctly parsed and validated.
24-24: LGTM!The change to use
resource.MustParse("8")for theCPUfield in theValidatorResourcestructure enhances type safety and ensures that the CPU resource is correctly parsed and validated.
25-25: LGTM!The change to use
resource.MustParse("20Gi")for theVolumefield in theValidatorResourcestructure enhances type safety and ensures that the volume resource is correctly parsed and validated.
28-28: LGTM!The change to use
resource.MustParse("1Gi")for theMemoryRequestfield in theTxClientsResourcestructure enhances type safety and ensures that the memory request is correctly parsed and validated.
29-29: LGTM!The change to use
resource.MustParse("3Gi")for theMemoryLimitfield in theTxClientsResourcestructure enhances type safety and ensures that the memory limit is correctly parsed and validated.
30-30: LGTM!The change to use
resource.MustParse("2")for theCPUfield in theTxClientsResourcestructure enhances type safety and ensures that the CPU resource is correctly parsed and validated.
31-31: LGTM!The change to use
resource.MustParse("1Gi")for theVolumefield in theTxClientsResourcestructure enhances type safety and ensures that the volume resource is correctly parsed and validated.
100-102: LGTM!The introduction of context handling using
context.WithCancel(context.Background())in theTwoNodeSimplefunction allows for better management of cancellation signals and timeouts during the benchmark test. Thecancelfunction is deferred to ensure proper cleanup.
105-105: LGTM!The change to update the
Cleanupmethod of thebenchTestobject to accept acontext.Contextparameter allows for better control over the cleanup process by utilizing the context for cancellation and timeout management.
110-110: LGTM!The change to update the
Runmethod of thebenchTestobject to accept acontext.Contextparameter allows for better control over the benchmark test execution by utilizing the context for cancellation and timeout management.
124-126: LGTM!The introduction of context handling using
context.WithCancel(context.Background())in therunBenchmarkTestfunction allows for better management of cancellation signals and timeouts during the benchmark test. Thecancelfunction is deferred to ensure proper cleanup.The changes to update the
CleanupandRunmethods of thebenchTestobject to accept the context parameter enable better control over the cleanup process and test execution by utilizing the context for cancellation and timeout management.Also applies to: 129-129, 133-133
test/e2e/benchmark/benchmark.go (3)
24-24: LGTM!The introduction of the
context.Contextparameter in theNewBenchmarkTestfunction aligns with the PR objective of enhancing context handling for better control over operation lifecycles. This change is consistent with the alterations mentioned in the AI-generated summary.
39-52: LGTM!The updates to the
SetupNodesfunction, including the introduction of thecontext.Contextparameter and the use of the provided context to create genesis nodes and obtain GRPC endpoints, align with the PR objective of enhancing context handling for better control over operation lifecycles. These changes are consistent with the alterations mentioned in the AI-generated summary and improve the ability to manage timeouts and cancellations.
101-130: LGTM!The modifications to the
Runfunction, including the introduction of thecontext.Contextparameter and the use of the provided context to start nodes and tx clients, align with the PR objective of enhancing context handling for better control over operation lifecycles. These changes are consistent with the alterations mentioned in the AI-generated summary and ensure that operations can be canceled or timed out appropriately. Additionally, the streamlined error handling with inline variable declarations improves readability.go.mod (8)
10-10: Verify compatibility with the updatedknuudependency.The version upgrade of
github.com/celestiaorg/knuufromv0.14.0tov0.15.2looks good. However, please ensure that you have thoroughly tested the integration with the updated dependency version to confirm that it doesn't introduce any breaking changes or unexpected behavior.
37-37: Ensure correct and efficient usage of the addedk8s.io/apimachinerydependency.The addition of the
k8s.io/apimachinerydependency with versionv0.30.2looks good. However, please ensure that the dependency is being used correctly and efficiently throughout the codebase to leverage its functionalities effectively.
93-93: LGTM!The version update of
github.com/go-ini/initov1.67.0looks good.
104-104: LGTM!The version update of
github.com/goccy/go-jsonfromv0.10.2tov0.10.3looks good.
146-147: LGTM!The version updates of
klauspost/compresstov1.17.9andklauspost/cpuid/v2tov2.2.8look good.
160-160: LGTM!The version update of
github.com/minio/minio-go/v7tov7.0.74looks good.
228-228: Verify compatibility with the updatedk8s.io/apidependency.The version upgrade of
k8s.io/apifromv0.28.2tov0.30.2looks good. However, please ensure that you have thoroughly tested the integration with the updated dependency version to confirm that it doesn't introduce any breaking changes or unexpected behavior.
229-229: Verify compatibility with the updatedk8s.io/client-godependency.The version upgrade of
k8s.io/client-gofromv0.28.2tov0.30.2looks good. However, please ensure that you have thoroughly tested the integration with the updated dependency version to confirm that it doesn't introduce any breaking changes or unexpected behavior.test/e2e/testnet/node.go (12)
Line range hint
101-181: LGTM!The changes to the
NewNodefunction improve its functionality and integration with theknuupackage. The addition of thecontext.Contextparameter allows for context-aware operations, while the new method chain simplifies the initialization of theknuu.Instance. The use of theresource.Quantitytype from Kubernetes enhances the precision of resource allocation.
183-186: LGTM!The
EnableNetShapermethod is a valuable addition to theNodestruct, allowing for the enabling of thenetshapersidecar. The implementation is correct and straightforward.
188-193: LGTM!The
SetLatencyAndJittermethod is a useful addition to theNodestruct, allowing for the setting of latency and jitter parameters for thenetshapersidecar. The implementation is correct, and the error handling ensures that thenetshapersidecar is enabled before setting the parameters.
Line range hint
195-274: LGTM!The changes to the
Initmethod improve its functionality by allowing for context-aware operations when committing theknuu.Instanceand adding the sidecars. The rest of the function remains largely unchanged and is correctly implemented.
Line range hint
280-289: LGTM!The changes to the
AddressP2Pmethod improve its functionality by allowing for context-aware operations when retrieving the IP address. The rest of the function remains unchanged and is correctly implemented.
306-312: LGTM!The changes to the
RemoteAddressGRPCmethod improve its functionality by allowing for context-aware operations when retrieving the IP address. The rest of the function remains unchanged and is correctly implemented.
Line range hint
315-321: LGTM!The changes to the
RemoteAddressRPCmethod improve its functionality by allowing for context-aware operations when retrieving the IP address. The rest of the function remains unchanged and is correctly implemented.
Line range hint
327-333: LGTM!The changes to the
RemoteAddressTracingmethod improve its functionality by allowing for context-aware operations when retrieving the IP address. The rest of the function remains unchanged and is correctly implemented.
344-350: LGTM!The changes to the
Startmethod improve its functionality by allowing for context-aware operations when starting theNodeinstance. The rest of the function remains unchanged and is correctly implemented.
352-354: LGTM!The changes to the
StartAsyncmethod improve its functionality by allowing for context-aware operations when starting theNodeinstance asynchronously. The rest of the function remains unchanged and is correctly implemented.
Line range hint
356-384: LGTM!The changes to the
WaitUntilStartedAndCreateProxymethod improve its functionality by allowing for context-aware operations when waiting for theNodeinstance to start and creating the necessary proxies. The rest of the function remains unchanged and is correctly implemented.
397-403: LGTM!The changes to the
Upgrademethod improve its functionality by allowing for context-aware operations when upgrading theNodeinstance to a new version. The rest of the function remains unchanged and is correctly implemented.test/e2e/testnet/testnet.go (14)
Line range hint
33-56: LGTM!The changes to the
Newfunction look good. The addition of thecontext.Contextparameter and the initialization of theknuuinstance with specific options enhance the testnet's ability to manage dependencies and configurations dynamically. The error handling for theknuuinstance creation is also properly implemented.
58-63: LGTM!The new
NewPreloadermethod looks good. It properly checks if theknuuinstance is initialized and returns an error if it's not. Creating a new preloader instance using theknuusystem dependencies ensures that theknuuinstance is properly utilized.
Line range hint
73-89: LGTM!The changes to the
CreateGenesisNodemethod look good. The addition of thecontext.Contextparameter and passing it to theNewNodefunction allows for context-aware operations, enabling the ability to cancel or timeout the node creation process. The rest of the function remains unchanged, which is appropriate.
91-98: LGTM!The changes to the
CreateGenesisNodesmethod look good. The addition of thecontext.Contextparameter and passing it to theCreateGenesisNodemethod allows for context-aware operations, enabling the ability to cancel or timeout the node creation process for multiple nodes. The rest of the function remains unchanged, which is appropriate.
Line range hint
100-124: LGTM!The changes to the
CreateTxClientsmethod look good. The addition of thecontext.Contextparameter and passing it to theCreateTxClientmethod allows for context-aware operations, enabling the ability to cancel or timeout the transaction client creation process for multiple clients. The rest of the function remains unchanged, which is appropriate.
Line range hint
136-186: LGTM!The changes to the
CreateTxClientmethod look good. The addition of thecontext.Contextparameter and passing it to theCreateTxClientfunction allows for context-aware operations, enabling the ability to cancel or timeout the transaction client creation process. Committing the transaction client instance using the context ensures that the operation can be properly canceled or timed out. The rest of the function remains unchanged, which is appropriate.
Line range hint
188-210: LGTM!The changes to the
StartTxClientsmethod look good. The addition of thecontext.Contextparameter and passing it to theStartAsyncandWaitInstanceIsRunningmethods allows for context-aware operations, enabling the ability to cancel or timeout the transaction client starting process for multiple clients. The rest of the function remains unchanged, which is appropriate.
255-268: LGTM!The changes to the
CreateNodemethod look good. The addition of thecontext.Contextparameter and passing it to theNewNodefunction allows for context-aware operations, enabling the ability to cancel or timeout the node creation process. The rest of the function remains unchanged, which is appropriate.
Line range hint
270-293: LGTM!The changes to the
Setupmethod look good. The addition of thecontext.Contextparameter and passing it to theAddressP2PandInitmethods allows for context-aware operations, enabling the ability to cancel or timeout the testnet setup process. The rest of the function remains unchanged, which is appropriate.
Line range hint
314-324: LGTM!The changes to the
RemoteGRPCEndpointsmethod look good. The addition of thecontext.Contextparameter and passing it to theRemoteAddressGRPCmethod allows for context-aware operations, enabling the ability to cancel or timeout the retrieval of remote gRPC endpoint addresses. The rest of the function remains unchanged, which is appropriate.
Line range hint
336-346: LGTM!The changes to the
RemoteRPCEndpointsmethod look good. The addition of thecontext.Contextparameter and passing it to theRemoteAddressRPCmethod allows for context-aware operations, enabling the ability to cancel or timeout the retrieval of remote RPC endpoint addresses. The rest of the function remains unchanged, which is appropriate.
Line range hint
350-384: LGTM!The changes to the
WaitToSyncmethod look good. The addition of thecontext.Contextparameter and passing it to theStatusmethod allows for context-aware operations, enabling the ability to cancel or timeout the waiting process for node synchronization. The rest of the function remains unchanged, which is appropriate.
Line range hint
389-416: LGTM!The changes to the
StartNodesmethod look good. The addition of thecontext.Contextparameter and passing it to theStartAsyncandWaitUntilStartedAndCreateProxymethods allows for context-aware operations, enabling the ability to cancel or timeout the node starting and proxy setup process. The rest of the function remains unchanged, which is appropriate.
418-432: LGTM!The changes to the
Startmethod look good. The addition of thecontext.Contextparameter and passing it to theStartNodes,WaitToSync, andStartTxClientsmethods allows for context-aware operations, enabling the ability to cancel or timeout the testnet starting process. The rest of the function remains unchanged, which is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
test/e2e/minor_version_compatibility.go (1)
Line range hint
20-114: Consider refactoring the function into smaller, more focused functions.The
MinorVersionCompatibilityfunction is quite large and could benefit from being broken down into smaller, more focused functions. This would improve readability and maintainability.For example, you could extract the logic for starting the nodes, upgrading the nodes, and checking the node heights into separate functions.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- test/e2e/main.go (4 hunks)
- test/e2e/minor_version_compatibility.go (3 hunks)
- test/e2e/simple.go (1 hunks)
- test/e2e/testnet/node.go (13 hunks)
Files skipped from review as they are similar to previous changes (3)
- test/e2e/main.go
- test/e2e/simple.go
- test/e2e/testnet/node.go
| testNet, err := testnet.New(ctx, "runMinorVersionCompatibility", seed, nil, "test") | ||
| testnet.NoError("failed to create testnet", err) | ||
|
|
||
| defer testNet.Cleanup(ctx) |
There was a problem hiding this comment.
Pass the context to the cleanup functions.
Please consider passing the ctx to the testNet.Cleanup and preloader.EmptyImages calls for consistency with the context usage throughout the function.
Apply this diff to pass the ctx:
- defer testNet.Cleanup(ctx)
+ defer func() { testnet.NoError("failed to cleanup testnet", testNet.Cleanup(ctx)) }()
- defer func() { _ = preloader.EmptyImages(ctx) }()
+ defer func() { testnet.NoError("failed to empty preloader images", preloader.EmptyImages(ctx)) }()Also applies to: 48-48
There was a problem hiding this comment.
this could be done in a followup issue.
test/e2e/main.go
Outdated
| // latestVersion, err := testnet.GetLatestVersion() | ||
| // testnet.NoError("failed to get latest version", err) |
There was a problem hiding this comment.
if this code is not used, can we delete it?
There was a problem hiding this comment.
we will use it once the BRR issue is resolved
There was a problem hiding this comment.
The --force-no-bbr flag exists on main but may not exist on v2.1.2
There was a problem hiding this comment.
thanks. the hard coded version removed after having the tests passed with the main. Since it was on a fork branch, the 07411e2 version from main is used.
Co-authored-by: CHAMI Rachid <[email protected]>
celestia-bot
requested review from
a team and
cmwaters
and removed request for
a team
|
@mojtaba-esk looks like test-e2e doesn't run on every PR. Can you run it manually locally and update the PR description with a testing section that states if it passes or fails. |
sure |
There was a problem hiding this comment.
Thanks @mojtaba-esk for integration of the new knuu! LGTM! One comment though:
In the previous version of knuu, there used to be a log message
Initializing knuu with scope: ...
that appeared when running the tests, indicating the test scope, which could then be used to tear down lingering instances or monitor the state of instances (e.g., via Lens). Currently, this log message is not showing up. Could you let me know how we can obtain this information i.e., the scope in the new version of knuu?
I also ran the following tests locally based on your branch and they worked for me:
- ✅
E2ESimple - ✅
MajorUpgradeToV2 - ✅
MinorVersionCompatibility:I am unable to run this one as I getUpdate: Could get a successful run ofcontext deadline exceedederror, but it might be a temporary thing.MinorVersionCompatibility✅ - ✅
TwoNodeBigBlock8MBLatencytogether with passing push configs (among all the e2e benchmark tests, testing this one seems sufficient to me to ensure almost everything works for all the other benchmark tests)
What about the rest of the e2e tests? Sanaz's comment makes it seem like |
|
Update: I was able to run |
oh yes, this is omitted from knuu logs by default. User can add it if they want it. Let me add that one quickly. |
mojtaba-esk
dismissed stale reviews from ninabarbakadze, staheri14, and rootulp
via
37c1778
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
test/e2e/testnet/testnet.go (1)
Line range hint
138-188: Approved with a suggestionThe changes to the
CreateTxClientmethod are comprehensive and well-structured. The addition of thecontext.Contextparameter enables context-aware operations throughout the process of creating a transaction client. The method handles the creation of the keyring directory, the creation of an account, and the initialization of the transaction client instance effectively.One suggestion for improvement:
Consider extracting the logic for creating the keyring directory and the account into separate helper methods. This would enhance the readability and maintainability of the
CreateTxClientmethod by reducing its complexity and making it more focused on the core functionality of creating the transaction client instance.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- test/e2e/testnet/testnet.go (19 hunks)
Additional comments not posted (11)
test/e2e/testnet/testnet.go (11)
33-49: LGTM!The changes to the
Newfunction look good. The addition of thecontext.Contextparameter and the initialization of theknuuinstance with specific options enhance the testnet's ability to manage dependencies and configurations dynamically.
60-65: Looks good!The new
NewPreloadermethod is a useful addition to theTestnetstruct. It ensures that theknuuinstance is initialized before creating the preloader instance, preventing potential issues. Using theknuusystem dependencies to create the preloader is a good approach for consistency and compatibility.
Line range hint
75-91: Approved!The changes to the
CreateGenesisNodemethod look good. The addition of thecontext.Contextparameter enhances the method's ability to handle context-aware operations. The logic for creating a genesis node remains clear and concise, generating the necessary keys and adding the node to the genesis validators and the testnet's list of nodes.
93-100: Changes look good!The modifications to the
CreateGenesisNodesmethod are straightforward and effective. The addition of thecontext.Contextparameter ensures that the method can handle context-aware operations when creating multiple genesis nodes. The method continues to iterate over the specified number of nodes and calls theCreateGenesisNodemethod with the provided configuration, maintaining its original functionality.
Line range hint
102-126: Looks good to me!The updates to the
CreateTxClientsmethod are well-structured and maintain the original functionality while incorporating thecontext.Contextparameter for context-aware operations. The method continues to iterate over the provided gRPC endpoints, calling theCreateTxClientmethod with the appropriate configuration for each endpoint. Error handling and logging are in place to handle any issues that may occur during the creation of transaction clients.
Line range hint
190-212: Changes look good!The modifications to the
StartTxClientsmethod are well-implemented and incorporate thecontext.Contextparameter effectively. The method maintains its original functionality of starting the transaction clients asynchronously and waiting for each client's instance to be running. The addition of thecontext.Contextparameter allows for context-aware operations during the start process.The error handling and logging mechanisms are in place to handle any issues that may occur during the start process and provide informative error messages.
257-270: LGTM!The updates to the
CreateNodemethod are straightforward and effective. The addition of thecontext.Contextparameter enables context-aware operations when creating a new node. The method continues to generate the necessary signer and network keys using the key generator and creates a new node with the provided configuration and the generated keys. The created node is then appended to the list of nodes in the testnet, maintaining the original functionality.
Line range hint
272-296: Looks good!The modifications to the
Setupmethod are well-structured and incorporate thecontext.Contextparameter effectively. The method maintains its original functionality of setting up the testnet by exporting the genesis configuration, retrieving the peer addresses for each node, and initializing each node with the exported genesis, peers, and provided configuration options.The addition of the
context.Contextparameter enables context-aware operations, particularly when retrieving the peer addresses using theAddressP2Pmethod, which now accepts the context parameter.
Line range hint
316-326: Changes look good!The updates to the
RemoteGRPCEndpointsmethod are straightforward and effective. The addition of thecontext.Contextparameter allows for context-aware operations when retrieving the remote gRPC endpoints of the testnet nodes. The method continues to iterate over the nodes, retrieving the remote gRPC endpoint for each node using theRemoteAddressGRPCmethod, which now accepts the context parameter.Error handling is in place to return any errors that may occur during the retrieval process. The retrieved gRPC endpoints are stored in a slice and returned, maintaining the original functionality of the method.
Line range hint
338-348: LGTM!The modifications to the
RemoteRPCEndpointsmethod are well-implemented and incorporate thecontext.Contextparameter effectively. The method maintains its original functionality of retrieving the remote RPC endpoints of the testnet nodes while adding context-aware operations.The method iterates over the nodes, retrieving the remote RPC endpoint for each node using the
RemoteAddressRPCmethod, which now accepts the context parameter. Error handling is in place to return any errors that may occur during the retrieval process. The retrieved RPC endpoints are stored in a slice and returned, preserving the expected behavior of the method.
Line range hint
352-386: Looks good!The updates to the
WaitToSyncmethod are well-structured and incorporate thecontext.Contextparameter effectively. The method maintains its original functionality of waiting for the testnet nodes to sync
|
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2024 Celestia Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
Closes #3780
Since this PR points to a branch on a fork,
GetLatestVersiondoes not fetch the latest commit hash to the main; instead, it fetches the latest commit of the forked repo. So it is tested with the hard coded version07411e2which is, currently, the latest commit on the main.The
E2ESimpletest passes, did not test the benchmarks which also were not needed for this purpose.